Skip to content

fix: decode API responses before exporting + CI verbosity#43

Merged
shreemaan-abhishek merged 3 commits into
masterfrom
create-json-decode-and-ci-verbosity
May 26, 2026
Merged

fix: decode API responses before exporting + CI verbosity#43
shreemaan-abhishek merged 3 commits into
masterfrom
create-json-decode-and-ci-verbosity

Conversation

@shreemaan-abhishek
Copy link
Copy Markdown
Contributor

@shreemaan-abhishek shreemaan-abhishek commented May 26, 2026

Two changes bundled — closes #32 and #41.

1. Decode API responses before exporting (closes #32)

Writing a json.RawMessage directly to yaml.v3 serializes the bytes as a list of integers, not a map. PR #31 fixed this for plugin-metadata get and PR #39's review fixup patched one stream-route create path, but the same anti-pattern lived in 21 other call sites across 12 resources.

Audit result:

Resource create path update path
route 🐛 🐛
service 🐛 🐛
consumer 🐛 🐛
credential 🐛 🐛
ssl 🐛 🐛
secret (file path elsewhere)
global-rule 🐛 🐛
proto 🐛 🐛
plugin-metadata 🐛 🐛
stream-route (already fixed) 🐛
gateway-group 🐛 🐛

All 21 sites silently produced byte-sequence YAML under -o yaml.

Fix:

  • New helper cmdutil.Exporter.WriteAPIResponse(body []byte) decodes once via encoding/json then writes through the existing Write() formatter.
  • All 21 call sites changed from .Write(json.RawMessage(body)).WriteAPIResponse(body).
  • Dropped now-unused encoding/json imports from consumer/create and consumer/update.
  • New unit tests on the helper:
    • TestExporter_WriteAPIResponse_YAML_DecodesObject — the regression guard (output must parse as a YAML map, not a byte sequence).
    • TestExporter_WriteAPIResponse_JSON_RoundTripsObject — JSON path still emits a proper object.
    • TestExporter_WriteAPIResponse_InvalidJSON_ReturnsError — clear error on undecodable body.

Verified live against API7 EE 3.9.12: service create -f -o yaml and route create -f -o yaml now emit YAML maps instead of integer sequences.

2. CI verbosity (closes #41)

Without -v, go test prints nothing between tests; combined with a silent TestMain (binary build + dashboard health-wait + group resolve), the CI log is dark for the first 2+ minutes of a healthy run.

  • Added -v to both make test-e2e and make test-e2e-full.
  • Added three progress markers to stderr in TestMain: "Building a7 binary ...", "Waiting for API7 EE dashboard at ...", "Resolving gateway group ...".

Verified live: progress markers fire in order, per-test --- PASS: lines stream out.

Test plan

  • go build, go vet, full go test ./... green locally.
  • New unit tests pass.
  • Live regression tests against API7 EE 3.9.12: service+route file-based -o yaml produce proper YAML maps.
  • Existing E2E tests (TestPluginMetadata_GetYAML, TestStreamRoute_CreateFromFileYAML, TestRoute_DescFlagCRUD) still pass.

Part of #22.

Summary by CodeRabbit

  • Tests

    • More verbose end-to-end setup logging for clearer progress
    • Added tests to validate decoded API response output in JSON/YAML and error on invalid responses
  • Refactor

    • Standardized how CLI commands export API responses for consistent formatting
  • Bug Fixes

    • Commands now surface decode failures instead of emitting raw response bytes for safer, clearer output

Review Change Stack

Writing a json.RawMessage directly to the yaml.v3 encoder serializes
the bytes as a list of integers, not a map. PR #31 fixed this for
`plugin-metadata get` and PR #39's review fixup did it for one
stream-route create path, but the same anti-pattern lived in 21 other
sites across 12 resources (every flag/file create and update path for
route, service, consumer, credential, ssl, secret, global-rule, proto,
plugin-metadata, stream-route, gateway-group).

- Add cmdutil.Exporter.WriteAPIResponse(body []byte) that decodes once
  via encoding/json and writes through the existing Write() formatter.
- Replace all 21 `.Write(json.RawMessage(body))` call sites with
  `.WriteAPIResponse(body)`.
- Drop now-unused encoding/json imports from consumer create/update.
- Unit tests for the helper cover YAML emits a map (the regression
  guard), JSON round-trips an object, and invalid JSON returns a
  clear error.

Verified live against API7 EE 3.9.12: service/route file-based create
`-o yaml` now emit proper maps instead of integer sequences.

Closes #32.
Without -v, `go test` prints nothing between tests; combined with a
silent TestMain (binary build + dashboard health-wait + group resolve)
the CI log shows nothing for the first 2+ minutes of a healthy run.

- Add -v to both `make test-e2e` and `make test-e2e-full` so each test
  emits === RUN / --- PASS lines as it runs.
- Print three progress markers to stderr in TestMain so the dark setup
  phase has visible breadcrumbs: building binary, waiting for dashboard,
  resolving gateway group.

Closes #41.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 46e51c3b-51fc-4f35-9af4-ed29441f1f57

📥 Commits

Reviewing files that changed from the base of the PR and between e0b4bad and 34a72ae.

📒 Files selected for processing (2)
  • pkg/cmd/ssl/create/create.go
  • pkg/cmd/ssl/update/update.go

📝 Walkthrough

Walkthrough

Adds Exporter.WriteAPIResponse to decode raw API JSON responses before formatting, applies it across CLI create/update file-based flows replacing json.RawMessage usage, adds tests for the exporter, and increases e2e test verbosity with setup progress messages.

Changes

API Response Exporter Fix and Application

Layer / File(s) Summary
Exporter WriteAPIResponse method and tests
pkg/cmdutil/exporter.go, pkg/cmdutil/exporter_test.go
New WriteAPIResponse(body []byte) error unmarshals raw JSON response bytes into a generic object and passes it to Write for JSON/YAML formatting; tests verify YAML decoding, JSON round-trip, and invalid input error handling.
Apply WriteAPIResponse across command create/update paths
pkg/cmd/*/create/*.go, pkg/cmd/*/update/*.go, pkg/cmd/ssl/*
Replace Write(json.RawMessage(body)) with WriteAPIResponse(body) in file-driven create/update flows across commands (consumer, credential, gateway-group, global-rule, plugin-metadata, proto, route, service, stream-route, plugin-metadata, etc.), and remove now-unused encoding/json imports; SSL commands now return a decode error when SSL JSON cannot be decoded.
E2E test verbosity and progress visibility
Makefile, test/e2e/setup_test.go
Add -v flag to Ginkgo invocation in test-e2e and test-e2e-full targets; insert three progress messages to stderr in TestMain setup to show build, dashboard health check, and gateway group resolution start.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
E2e Test Quality Review ⚠️ Warning PR fails E2E completeness: 17 files fixed but only 1 E2E regression test (stream_route) added; issue #32 requires tests for all fixed resources covering file-based YAML output. Add E2E tests for route, service, consumer, credential, ssl, proto, global-rule, plugin-metadata, gateway-group following TestStreamRoute_CreateFromFileYAML pattern to verify file-based -o yaml works correctly.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the two main changes: fixing API response decoding for export (closes #32) and adding CI verbosity (closes #41).
Linked Issues check ✅ Passed All objectives from #32 and #41 are met: WriteAPIResponse method added, all 21 json.RawMessage sites replaced, unit tests added, CI verbosity enhanced with -v flag and stderr progress markers.
Out of Scope Changes check ✅ Passed All changes are in scope: API response decoding refactoring across 12 resource types, SSL error handling alignment, and CI verbosity improvements align with objectives.
Security Check ✅ Passed Decodes JSON responses before export; improves SSL with fail-closed error handling. No issues across all seven security categories reviewed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch create-json-decode-and-ci-verbosity

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/cmd/ssl/create/create.go`:
- Line 159: The current code falls back to
cmdutil.NewExporter(...).WriteAPIResponse(body) when decoding api.SSL fails,
risking unredacted secret fields; change the control flow so that if decoding
api.SSL returns an error you do NOT call WriteAPIResponse(body) — instead
return/propagate the decode error (or send a generic error response) and only
call WriteAPIResponse after successfully decoding and applying api.RedactSSL on
the SSL struct; specifically, locate the branch where api.SSL is decoded and
replace the fallback to WriteAPIResponse(body) with error handling (or a
sanitized response) and ensure you call api.RedactSSL(&ssl) before any
serialization.

In `@pkg/cmd/ssl/update/update.go`:
- Line 182: The fallback path currently calls cmdutil.NewExporter(format,
out).WriteAPIResponse(body) which can export unredacted SSL secrets; update the
fallback to first call api.RedactSSL on the response payload (or construct a
redacted copy) and pass that redacted value to WriteAPIResponse so cert/key
material is never serialized, ensuring any SSL-bearing struct uses its
Redact/Marshal implementations; look for WriteAPIResponse, cmdutil.NewExporter,
and api.RedactSSL in the update handler and replace the direct
WriteAPIResponse(body) usage with a call that redacts (or converts to a redacted
struct) before exporting.

In `@pkg/cmdutil/exporter_test.go`:
- Line 1: This new Go test file is missing the project Apache 2.0 license
header; add the standard Apache 2.0 source header comment block at the very top
of the file above the existing "package cmdutil" declaration so the file begins
with the license header followed by the package statement, ensuring formatting
and year/owner placeholders follow the project's header template.

In `@pkg/cmdutil/exporter.go`:
- Around line 42-47: The Exporter.WriteAPIResponse currently uses json.Unmarshal
into an interface{}, which converts numbers to float64 and can lose precision;
change it to create a json.Decoder on an io.Reader (e.g.,
bytes.NewReader(body)), call decoder.UseNumber() before Decode(&decoded) so
numeric values are preserved as json.Number, then return e.Write(decoded) as
before; also add a unit test that sends a JSON payload with an integer > 2^53
through WriteAPIResponse and asserts the exported output preserves the exact
integer string to catch regressions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ee665c17-ce9a-4499-89f0-ecacc6fad52e

📥 Commits

Reviewing files that changed from the base of the PR and between 3fbbb89 and e0b4bad.

📒 Files selected for processing (23)
  • Makefile
  • pkg/cmd/consumer/create/create.go
  • pkg/cmd/consumer/update/update.go
  • pkg/cmd/credential/create/create.go
  • pkg/cmd/credential/update/update.go
  • pkg/cmd/gateway-group/create/create.go
  • pkg/cmd/gateway-group/update/update.go
  • pkg/cmd/global-rule/create/create.go
  • pkg/cmd/global-rule/update/update.go
  • pkg/cmd/plugin-metadata/create/create.go
  • pkg/cmd/plugin-metadata/update/update.go
  • pkg/cmd/proto/create/create.go
  • pkg/cmd/proto/update/update.go
  • pkg/cmd/route/create/create.go
  • pkg/cmd/route/update/update.go
  • pkg/cmd/service/create/create.go
  • pkg/cmd/service/update/update.go
  • pkg/cmd/ssl/create/create.go
  • pkg/cmd/ssl/update/update.go
  • pkg/cmd/stream-route/update/update.go
  • pkg/cmdutil/exporter.go
  • pkg/cmdutil/exporter_test.go
  • test/e2e/setup_test.go

Comment thread pkg/cmd/ssl/create/create.go Outdated
Comment thread pkg/cmd/ssl/update/update.go Outdated
Comment thread pkg/cmdutil/exporter_test.go
Comment thread pkg/cmdutil/exporter.go
The previous fallback dumped the raw response body via WriteAPIResponse,
bypassing api.RedactSSL and exporting the private key in plaintext when
the body didn't decode into api.SSL. Return an error in that path so the
key is never serialized unredacted.
@shreemaan-abhishek shreemaan-abhishek merged commit 2f453fc into master May 26, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci(e2e): improve CI verbosity so the suite isn't dark for minutes Audit *create.go files for json.RawMessage exporter bug

1 participant